On 08.09.21 13:03, Jan Beulich wrote:
> On 08.09.2021 11:43, Oleksandr Andrushchenko wrote:
>> On 08.09.21 12:27, Jan Beulich wrote:
>>> On 07.09.2021 19:39, Oleksandr Andrushchenko wrote:
>>>> On 07.09.21 19:30, Jan Beulich wrote:
>>>>> On 07.09.2021 15:33, Oleksandr Andrushchenko wrote:
>>>>>> On 06.09.21 17:31, Jan Beulich wrote:
>>>>>>> On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
>>>>>>>> --- a/xen/drivers/vpci/header.c
>>>>>>>> +++ b/xen/drivers/vpci/header.c
>>>>>>>> @@ -400,12 +400,72 @@ static void bar_write(const struct pci_dev
>>>>>>>> *pdev, unsigned int reg,
>>>>>>>> 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;
>>>>>>>> + bar->guest_addr &= ~(0xffffffffull << (hi ? 32 : 0));
>>>>>>>> + bar->guest_addr |= (uint64_t)val << (hi ? 32 : 0);
>>>>>>> What you store here is not the address that's going to be used,
>>>>>>> as
>>>>>>> you don't mask off the low bits (to account for the BAR's size).
>>>>>>> When a BAR gets written with all ones, all writable bits get these
>>>>>>> ones stored. The address of the BAR, aiui, really changes to
>>>>>>> (typically) close below 4Gb (in the case of a 32-bit BAR), which
>>>>>>> is why memory / I/O decoding should be off while sizing BARs.
>>>>>>> Therefore you shouldn't look for the specific "all writable bits
>>>>>>> are ones" pattern (or worse, as you presently do, the "all bits
>>>>>>> outside of the type specifier are ones" one) on the read path.
>>>>>>> Instead mask the value appropriately here, and simply return back
>>>>>>> the stored value from the read path.
>> But in case of BAR sizing I need to actually return BAR size.
>> So, the comparison is the way to tell if the guest wants to read
>> actual (configured) BAR value or it tries to determine BAR's size.
>> This is why I compare and use the result as the answer to what needs
>> to be supplied to the guest. So, if I don't compare with 0xffffffff for the
>> hi part and 0xfffffff0 for the low then how do I know when to return
>> configured BAR or return the size?
> Well, but that's the common misunderstanding that I've been trying
> to point out: There's no difference between these two forms of
> reads. The BARs are simply registers with some r/o bits. There's
> no hidden 2nd register recording what was last written. When you
> write 0xffffffff, all you do is set all writable bits to 1. When
> you read back from the register, you will find all r/o bits
> unchanged (which in particular means all lower address bits are
> zero, thus allowing you to determine the size).
>
> When the spec says to write 0xffffffff for sizing purposes, OSes
> should follow that, yes. This doesn't preclude them to use other
> forms of writes for whichever purpose. Hence you do not want to
> special case sizing, but instead you want to emulate correctly
> all forms of writes, including subsequent reads to uniformly
> return the intended / expected values.
>
> Just to give an example (perhaps a little contrived): To size a
> 64-bit BAR, in principle you'd first need to write 0xffffffff to
> both halves. But there's nothing wrong with doing this in a
> different order: Act on the low half alone first, and then act
> on the high half. The acting on the high half could even be
> skipped if the low half sizing produced at least bit 31 set. Now
> if you were to special case seeing ffffffff:fffffff? as the
> last written pair of values, you'd break that (imo legitimate)
> alternative process of sizing.
How about:
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)
{
struct vpci_bar *bar = data;
if ( bar->type == VPCI_BAR_MEM64_HI )
return bar->guest_addr >> 32;
return bar->guest_addr;
}
It seems to solve all the questions we have: more work on write path,
no comparison with 0xffffffff: BAR's size is used to mask unwanted bits.
BTW, bars[i].guest_addr = 0; is needed as this field can be re-used.
>
> Jan
>
Thank you,
Oleksandr
