Re: [PATCH v4 05/11] vpci/header: implement guest BAR register handlers

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

On Tue, Nov 23, 2021 at 03:14:27PM +0000, Oleksandr Andrushchenko wrote:
> Hi, Roger!
> 
> On 19.11.21 15:02, Jan Beulich wrote:
> > On 19.11.2021 13:54, Oleksandr Andrushchenko wrote:
> >> On 19.11.21 14:49, Jan Beulich wrote:
> >>> On 19.11.2021 13:46, Oleksandr Andrushchenko wrote:
> >>>> On 19.11.21 14:37, Jan Beulich wrote:
> >>>>> On 19.11.2021 13:10, Oleksandr Andrushchenko wrote:
> >>>>>> On 19.11.21 13:58, Jan Beulich wrote:
> >>>>>>> On 05.11.2021 07:56, Oleksandr Andrushchenko wrote:
> >>>>>>>> --- a/xen/drivers/vpci/header.c
> >>>>>>>> +++ b/xen/drivers/vpci/header.c
> >>>>>>>> @@ -408,6 +408,48 @@ static void bar_write(const struct pci_dev 
> >>>>>>>> *pdev, unsigned int reg,
> >>>>>>>>          pci_conf_write32(pdev->sbdf, reg, val);
> >>>>>>>>      }
> >>>>>>>>      
> >>>>>>>> +static void guest_bar_write(const struct pci_dev *pdev, unsigned 
> >>>>>>>> int reg,
> >>>>>>>> +                            uint32_t val, void *data)
> >>>>>>>> +{
> >>>>>>>> +    struct vpci_bar *bar = data;
> >>>>>>>> +    bool hi = false;
> >>>>>>>> +
> >>>>>>>> +    if ( bar->type == VPCI_BAR_MEM64_HI )
> >>>>>>>> +    {
> >>>>>>>> +        ASSERT(reg > PCI_BASE_ADDRESS_0);
> >>>>>>>> +        bar--;
> >>>>>>>> +        hi = true;
> >>>>>>>> +    }
> >>>>>>>> +    else
> >>>>>>>> +    {
> >>>>>>>> +        val &= PCI_BASE_ADDRESS_MEM_MASK;
> >>>>>>>> +        val |= bar->type == VPCI_BAR_MEM32 ? 
> >>>>>>>> PCI_BASE_ADDRESS_MEM_TYPE_32
> >>>>>>>> +                                           : 
> >>>>>>>> PCI_BASE_ADDRESS_MEM_TYPE_64;
> >>>>>>>> +        val |= bar->prefetchable ? PCI_BASE_ADDRESS_MEM_PREFETCH : 
> >>>>>>>> 0;
> >>>>>>>> +    }
> >>>>>>>> +
> >>>>>>>> +    bar->guest_addr &= ~(0xffffffffull << (hi ? 32 : 0));
> >>>>>>>> +    bar->guest_addr |= (uint64_t)val << (hi ? 32 : 0);
> >>>>>>>> +
> >>>>>>>> +    bar->guest_addr &= ~(bar->size - 1) | 
> >>>>>>>> ~PCI_BASE_ADDRESS_MEM_MASK;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned 
> >>>>>>>> int reg,
> >>>>>>>> +                               void *data)
> >>>>>>>> +{
> >>>>>>>> +    const struct vpci_bar *bar = data;
> >>>>>>>> +    bool hi = false;
> >>>>>>>> +
> >>>>>>>> +    if ( bar->type == VPCI_BAR_MEM64_HI )
> >>>>>>>> +    {
> >>>>>>>> +        ASSERT(reg > PCI_BASE_ADDRESS_0);
> >>>>>>>> +        bar--;
> >>>>>>>> +        hi = true;
> >>>>>>>> +    }
> >>>>>>>> +
> >>>>>>>> +    return bar->guest_addr >> (hi ? 32 : 0);
> >>>>>>> I'm afraid "guest_addr" then isn't the best name; maybe "guest_val"?
> >>>>>>> This would make more obvious that there is a meaningful difference
> >>>>>>> from "addr" besides the guest vs host aspect.
> >>>>>> I am not sure I can agree here:
> >>>>>> bar->addr and bar->guest_addr make it clear what are these while
> >>>>>> bar->addr and bar->guest_val would make someone go look for
> >>>>>> additional information about what that val is for.
> >>>>> Feel free to replace "val" with something more suitable. "guest_bar"
> >>>>> maybe? The value definitely is not an address, so "addr" seems
> >>>>> inappropriate / misleading to me.
> >>>> This is a guest's view on the BAR's address. So to me it is still 
> >>>> guest_addr
> >>> It's a guest's view on the BAR, not just the address. Or else you couldn't
> >>> simply return the value here without folding in the correct low bits.
> >> I agree with this this respect as it is indeed address + lower bits.
> >> How about guest_bar_val then? So it reflects its nature, e.g. the value
> >> of the BAR as seen by the guest.
> > Gets a little longish for my taste. I for one wouldn't mind it be just
> > "guest". In the end Roger has the final say here anyway.
> What is your preference on naming here?
> 1. guest_addr
> 2. guest_val
> 3. guest_bar_val
> 4. guest

I think guest_reg would be fine?

Or alternatively you could make it a guest address by dropping the low
bits and adding them in the read handler instead of doing it in the
write handler.

Thanks, Roger.