Message ID | 20211105065629.940943-6-andr2000@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | PCI devices passthrough on Arm, part 3 | expand |
On 05.11.2021 07:56, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > Add relevant vpci register handlers when assigning PCI device to a domain > and remove those when de-assigning. This allows having different > handlers for different domains, e.g. hwdom and other guests. > > Emulate guest BAR register values: this allows creating a guest view > of the registers and emulates size and properties probe as it is done > during PCI device enumeration by the guest. > > ROM BAR is only handled for the hardware domain and for guest domains > there is a stub: at the moment PCI expansion ROM is x86 only, so it > might not be used by other architectures without emulating x86. Other > use-cases may include using that expansion ROM before Xen boots, hence > no emulation is needed in Xen itself. Or when a guest wants to use the > ROM code which seems to be rare. At least in the initial days of EFI there was the concept of EFI byte code, for ROM code to be compiled to such that it would be arch- independent. While I don't mean this to be an argument against leaving out ROM BAR handling for now, this may want mentioning here to avoid giving the impression that it's only x86 which might be affected by this deliberate omission. > --- 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. Jan
On 19.11.21 13:58, Jan Beulich wrote: > On 05.11.2021 07:56, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> Add relevant vpci register handlers when assigning PCI device to a domain >> and remove those when de-assigning. This allows having different >> handlers for different domains, e.g. hwdom and other guests. >> >> Emulate guest BAR register values: this allows creating a guest view >> of the registers and emulates size and properties probe as it is done >> during PCI device enumeration by the guest. >> >> ROM BAR is only handled for the hardware domain and for guest domains >> there is a stub: at the moment PCI expansion ROM is x86 only, so it >> might not be used by other architectures without emulating x86. Other >> use-cases may include using that expansion ROM before Xen boots, hence >> no emulation is needed in Xen itself. Or when a guest wants to use the >> ROM code which seems to be rare. > At least in the initial days of EFI there was the concept of EFI byte > code, for ROM code to be compiled to such that it would be arch- > independent. While I don't mean this to be an argument against leaving > out ROM BAR handling for now, this may want mentioning here to avoid > giving the impression that it's only x86 which might be affected by > this deliberate omission. I can put: at the moment PCI expansion ROM handling is supported for x86 only and it might not be used by other architectures without emulating x86. > >> --- 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. > > Jan > Thank you, Oleksandr
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: >>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>> >>> Add relevant vpci register handlers when assigning PCI device to a domain >>> and remove those when de-assigning. This allows having different >>> handlers for different domains, e.g. hwdom and other guests. >>> >>> Emulate guest BAR register values: this allows creating a guest view >>> of the registers and emulates size and properties probe as it is done >>> during PCI device enumeration by the guest. >>> >>> ROM BAR is only handled for the hardware domain and for guest domains >>> there is a stub: at the moment PCI expansion ROM is x86 only, so it >>> might not be used by other architectures without emulating x86. Other >>> use-cases may include using that expansion ROM before Xen boots, hence >>> no emulation is needed in Xen itself. Or when a guest wants to use the >>> ROM code which seems to be rare. >> At least in the initial days of EFI there was the concept of EFI byte >> code, for ROM code to be compiled to such that it would be arch- >> independent. While I don't mean this to be an argument against leaving >> out ROM BAR handling for now, this may want mentioning here to avoid >> giving the impression that it's only x86 which might be affected by >> this deliberate omission. > I can put: > at the moment PCI expansion ROM handling is supported for x86 only > and it might not be used by other architectures without emulating x86. Sounds at least somewhat better to me. >>> --- 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. Jan
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: >>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>>> >>>> Add relevant vpci register handlers when assigning PCI device to a domain >>>> and remove those when de-assigning. This allows having different >>>> handlers for different domains, e.g. hwdom and other guests. >>>> >>>> Emulate guest BAR register values: this allows creating a guest view >>>> of the registers and emulates size and properties probe as it is done >>>> during PCI device enumeration by the guest. >>>> >>>> ROM BAR is only handled for the hardware domain and for guest domains >>>> there is a stub: at the moment PCI expansion ROM is x86 only, so it >>>> might not be used by other architectures without emulating x86. Other >>>> use-cases may include using that expansion ROM before Xen boots, hence >>>> no emulation is needed in Xen itself. Or when a guest wants to use the >>>> ROM code which seems to be rare. >>> At least in the initial days of EFI there was the concept of EFI byte >>> code, for ROM code to be compiled to such that it would be arch- >>> independent. While I don't mean this to be an argument against leaving >>> out ROM BAR handling for now, this may want mentioning here to avoid >>> giving the impression that it's only x86 which might be affected by >>> this deliberate omission. >> I can put: >> at the moment PCI expansion ROM handling is supported for x86 only >> and it might not be used by other architectures without emulating x86. > Sounds at least somewhat better to me. > >>>> --- 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 > > Jan > Thank you, Oleksandr
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. Jan
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. > > Jan > Thank you, Oleksandr
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. Jan
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. Ok, so let Roger chose the name ;) > > Jan > Thank you, Oleksandr
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 > > Jan > Thank you in advance, Oleksandr
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.
On 24.11.21 14:32, Roger Pau Monné wrote: > 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. So, let it be "guest_reg" then > > Thanks, Roger. Thank you, Oleksandr
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index ef538386e95d..1239051ee8ff 100644 --- 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); +} + static void rom_write(const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data) { @@ -456,6 +498,17 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg, rom->addr = val & PCI_ROM_ADDRESS_MASK; } +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 init_bars(struct pci_dev *pdev) { uint16_t cmd; @@ -464,6 +517,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 ) { @@ -503,8 +557,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); @@ -544,8 +600,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); @@ -565,8 +623,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); if ( rc ) rom->type = VPCI_BAR_EMPTY; } diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h index a016b4197801..3e7428da822c 100644 --- a/xen/include/xen/vpci.h +++ b/xen/include/xen/vpci.h @@ -70,7 +70,10 @@ struct vpci { struct vpci_header { /* Information about the PCI BARs of this device. */ struct vpci_bar { + /* Physical view of the BAR. */ uint64_t addr; + /* Guest view of the BAR. */ + uint64_t guest_addr; uint64_t size; enum { VPCI_BAR_EMPTY,