Re: [PATCH v5 06/14] vpci/header: implement guest BAR register handlers

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

On Mon, Jan 31, 2022 at 09:47:07AM +0000, Oleksandr Andrushchenko wrote:
> Hi, Roger!
> 
> On 12.01.22 14:35, Roger Pau Monné wrote:
> > On Thu, Nov 25, 2021 at 01:02:43PM +0200, Oleksandr Andrushchenko wrote:
> >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> >> +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;
> >> +}
> > There should be no need for those handlers. As said elsewhere: for
> > guests registers not explicitly handled should return ~0 for reads and
> > drop writes, which is what you are proposing here.
> Yes, you are right: I can see in vpci_read that we end up reading ~0 if no
> handler exists (which is what I do here with guest_rom_read). But I am not 
> that
> sure about the dropped writes:
> 
> void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
>                  uint32_t data)
> {
>      unsigned int data_offset = 0;
> 
> [snip]
> 
>      if ( data_offset < size )
>          /* Tailing gap, write the remaining. */
>          vpci_write_hw(sbdf, reg + data_offset, size - data_offset,
>                        data >> (data_offset * 8));
> 
> so it looks like for the un-handled writes we still reach the HW register.
> Could you please tell if the code above needs improvement (like checking
> if the write was handled) or I still need to provide a write handler, e.g.
> guest_rom_write here?

Right now (given the current code) unhandled reads and writes will all
end up being forwarded to the hardware. This is intended for dom0, but
this is not how it's going to work for domUs, where accesses will be
discarded based on an accept list. IOW the handlers that you are
adding here should be the default behavior for registers not
explicitly handled in the domU case, and shouldn't require explicit
handling.

> >> +
> >>   static int init_bars(struct pci_dev *pdev)
> >>   {
> >>       uint16_t cmd;
> >> @@ -489,6 +542,7 @@ static int init_bars(struct pci_dev *pdev)
> >>       struct vpci_header *header = &pdev->vpci->header;
> >>       struct vpci_bar *bars = header->bars;
> >>       int rc;
> >> +    bool is_hwdom = is_hardware_domain(pdev->domain);
> >>   
> >>       switch ( pci_conf_read8(pdev->sbdf, PCI_HEADER_TYPE) & 0x7f )
> >>       {
> >> @@ -528,8 +582,10 @@ static int init_bars(struct pci_dev *pdev)
> >>           if ( i && bars[i - 1].type == VPCI_BAR_MEM64_LO )
> >>           {
> >>               bars[i].type = VPCI_BAR_MEM64_HI;
> >> -            rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, 
> >> reg,
> >> -                                   4, &bars[i]);
> >> +            rc = vpci_add_register(pdev->vpci,
> >> +                                   is_hwdom ? vpci_hw_read32 : 
> >> guest_bar_read,
> >> +                                   is_hwdom ? bar_write : guest_bar_write,
> >> +                                   reg, 4, &bars[i]);
> >>               if ( rc )
> >>               {
> >>                   pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> >> @@ -569,8 +625,10 @@ static int init_bars(struct pci_dev *pdev)
> >>           bars[i].size = size;
> >>           bars[i].prefetchable = val & PCI_BASE_ADDRESS_MEM_PREFETCH;
> >>   
> >> -        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, 
> >> reg, 4,
> >> -                               &bars[i]);
> >> +        rc = vpci_add_register(pdev->vpci,
> >> +                               is_hwdom ? vpci_hw_read32 : guest_bar_read,
> >> +                               is_hwdom ? bar_write : guest_bar_write,
> >> +                               reg, 4, &bars[i]);
> >>           if ( rc )
> >>           {
> >>               pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> >> @@ -590,8 +648,10 @@ static int init_bars(struct pci_dev *pdev)
> >>           header->rom_enabled = pci_conf_read32(pdev->sbdf, rom_reg) &
> >>                                 PCI_ROM_ADDRESS_ENABLE;
> >>   
> >> -        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write, 
> >> rom_reg,
> >> -                               4, rom);
> >> +        rc = vpci_add_register(pdev->vpci,
> >> +                               is_hwdom ? vpci_hw_read32 : guest_rom_read,
> >> +                               is_hwdom ? rom_write : guest_rom_write,
> >> +                               rom_reg, 4, rom);
> > This whole call should be made conditional to is_hwdom, as said above
> > there's no need for the guest_rom handlers.
> Yes, if writes are indeed dropped, please see question above
> >
> > Likewise I assume you expect IO BARs to simply return ~0 and drop
> > writes, as there's no explicit handler added for those?
> Yes, but that was not my intention: I simply didn't handle IO BARs
> and now we do need that handling: either with the default behavior
> for the unhandled read/write (drop writes, read ~0) or by introducing
> the handlers. I hope we can rely on the "unhandled read/write" and
> get what we want

Indeed, the default behavior should be changed for domUs to drop
writes, return ~0 for unhandled reads, then you won't need to add
dummy handlers for the registers you don't want to expose.

> >
> >>           if ( rc )
> >>               rom->type = VPCI_BAR_EMPTY;
> >>       }
> >> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> >> index ed127a08a953..0a73b14a92dc 100644
> >> --- a/xen/include/xen/vpci.h
> >> +++ b/xen/include/xen/vpci.h
> >> @@ -68,7 +68,10 @@ struct vpci {
> >>       struct vpci_header {
> >>           /* Information about the PCI BARs of this device. */
> >>           struct vpci_bar {
> >> +            /* Physical view of the BAR. */
> > No, that's not the physical view, it's the physical (host) address.
> Ok
> >
> >>               uint64_t addr;
> >> +            /* Guest view of the BAR: address and lower bits. */
> >> +            uint64_t guest_reg;
> > I continue to think it would be clearer if you store the guest address
> > here (gaddr, without the low bits) and add those in guest_bar_read
> > based on bar->{type,prefetchable}. Then it would be equivalent to the
> > existing 'addr' field.
> Ok, I'll re-work the code with this approach in mind: s/guest_reg/gaddr +
> required code to handle that
> >
> > I wonder whether we need to protect the added code with
> > CONFIG_HAS_VPCI_GUEST_SUPPORT, this would effectively be dead code
> > otherwise. Long term I don't think we wish to differentiate between
> > dom0 and domU vPCI support at build time, so I'm unsure whether it's
> > helpful to pollute the code with CONFIG_HAS_VPCI_GUEST_SUPPORT when
> > the plan is to remove those long term.
> I would have it without CONFIG_HAS_VPCI_GUEST_SUPPORT if you
> don't mind

Well, I guess if it's not too intrusive it's fine to add the defines,
removing them afterwards should be easy.

Thanks, Roger.