Re: [PATCH v3 04/11] vpci/header: Add and remove register handlers dynamically

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Oct 07, 2021 at 09:22:36AM +0200, Jan Beulich wrote:
> On 04.10.2021 07:58, Oleksandr Andrushchenko wrote:
> > 
> > 
> > On 01.10.21 16:26, Jan Beulich wrote:
> >> On 30.09.2021 09:52, Oleksandr Andrushchenko wrote:
> >>> @@ -445,14 +456,25 @@ static void rom_write(const struct pci_dev *pdev, 
> >>> unsigned int reg,
> >>>           rom->addr = val & PCI_ROM_ADDRESS_MASK;
> >>>   }
> >>>   
> >>> -static int add_bar_handlers(const struct pci_dev *pdev)
> >>> +static void guest_rom_write(const struct pci_dev *pdev, unsigned int reg,
> >>> +                            uint32_t val, void *data)
> >>> +{
> >>> +}
> >>> +
> >>> +static uint32_t guest_rom_read(const struct pci_dev *pdev, unsigned int 
> >>> reg,
> >>> +                               void *data)
> >>> +{
> >>> +    return 0xffffffff;
> >>> +}
> >>> +
> >>> +static int add_bar_handlers(const struct pci_dev *pdev, bool is_hwdom)
> >> I remain unconvinced that this boolean is the best way to go here,
> > I can remove "bool is_hwdom" and have the checks like:
> > 
> > static int add_bar_handlers(const struct pci_dev *pdev)
> > {
> > ...
> >      if ( is_hardware_domain(pdev->domain) )
> >          rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write,
> >                                 PCI_COMMAND, 2, header);
> >      else
> >          rc = vpci_add_register(pdev->vpci, vpci_hw_read16, guest_cmd_write,
> >                                 PCI_COMMAND, 2, header);
> > Is this going to be better?
> 
> Marginally (plus you'd need to prove that pdev->domain can never be NULL
> when making it here).

I think it would an anomaly to try to setup vPCI handlers for a device
without pdev->domain being set. I'm quite sure other vPCI code already
relies on pdev->domain being set.

As I said in another reply I'm not convinced though that splitting
add_bar_handlers is the right thing to do.

Thanks, Roger.