On Tue, Feb 08, 2022 at 10:16:59AM +0100, Jan Beulich wrote:
> On 08.02.2022 09:06, Oleksandr Andrushchenko wrote:
> >
> >
> > On 07.02.22 19:06, Jan Beulich wrote:
> >> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
> >>> +static uint32_t guest_bar_ignore_read(const struct pci_dev *pdev,
> >>> + unsigned int reg, void *data)
> >>> +{
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int bar_ignore_access(const struct pci_dev *pdev, unsigned int
> >>> reg,
> >>> + struct vpci_bar *bar)
> >>> +{
> >>> + if ( is_hardware_domain(pdev->domain) )
> >>> + return 0;
> >>> +
> >>> + return vpci_add_register(pdev->vpci, guest_bar_ignore_read, NULL,
> >>> + reg, 4, bar);
> >>> +}
> >> For these two functions: I'm not sure "ignore" is an appropriate
> >> term here. unused_bar_read() and unused_bar() maybe? Or,
> >> considering we already have VPCI_BAR_EMPTY, s/unused/empty/ ? I'm
> >> also not sure we really need the is_hardware_domain() check here:
> >> Returning 0 for Dom0 is going to be fine as well; there's no need
> >> to fetch the value from actual hardware. The one exception might
> >> be for devices with buggy BAR behavior ...
> > Well, I think this should be ok, so then
> > - s/guest_bar_ignore_read/empty_bar_read
> > - s/bar_ignore_access/empty_bar
>
> Hmm, seeing it, I don't think empty_bar() is a good function name.
> setup_empty_bar() or empty_bar_setup() would make more clear what
> the function's purpose is.
I don't think you require an empty_bar_setup helper, the code there is
trivial can be open coded in init_bars directly IMO.
>
> > - no is_hardware_domain check
>
> Please wait a little to see whether Roger has any input on this aspect.
I think for the hw domain we should allow access to the BAR even if Xen
has found it empty. Adding the ignore handlers for dom0 shouldn't make
any difference, but we never know whether some quirky hardware could
make use of that.
Thanks, Roger.
