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.
