Hi,
On 5/12/20 3:17 PM, André Przywara wrote:
> On 06/05/2020 14:51, Alexandru Elisei wrote:
>
> Hi,
>
>> On 4/6/20 3:06 PM, André Przywara wrote:
>>> On 26/03/2020 15:24, Alexandru Elisei wrote:
>>>
>>>>
>>>> union pci_config_address {
>>>> struct {
>>>> @@ -58,6 +91,8 @@ union pci_config_address {
>>>> };
>>>> u32 w;
>>>> };
>>>> +#endif
>>>> +#define PCI_DEV_CFG_MASK (PCI_DEV_CFG_SIZE - 1)
>>>>
>>>> struct msix_table {
>>>> struct msi_msg msg;
>>>> @@ -100,6 +135,33 @@ struct pci_cap_hdr {
>>>> u8 next;
>>>> };
>>>>
>>>> +struct pcie_cap {
>>> I guess this is meant to map to the PCI Express Capability Structure as
>>> described in the PCIe spec?
>>> We would need to add the "packed" attribute then. But actually I am not
>>> a fan of using C language constructs to model specified register
>>> arrangements, the kernel tries to avoid this as well.
>> I'm not sure what you are suggesting. Should we rewrite the entire PCI emulation
>> code in kvmtool then?
> At least not add more of that problematic code, especially if we don't
I don't see why how the code is problematic. Did I miss it in a previous comment?
> need it. Maybe there is a better solution for the operations we will
> need (byte array?), that's hard to say without seeing the code.
>
>>> Actually, looking closer: why do we need this in the first place? I
>>> removed this and struct pm_cap, and it still compiles.
>>> So can we lose those two structures at all? And move the discussion and
>>> implementation (for VirtIO 1.0?) to a later series?
>> I've answered both points in v2 of the series [1].
>>
>> [1] https://www.spinics.net/lists/kvm/msg209601.html:
> From there:
>>> But more importantly: Do we actually need those definitions? We
>>> don't seem to use them, do we?
>>> And the u8 __pad[PCI_DEV_CFG_SIZE] below should provide the extended
>>> storage space a guest would expect?
>> Yes, we don't use them for the reasons I explained in the commit
>> message. I would rather keep them, because they are required by the
>> PCIE spec.
> I don't get the point of adding code / data structures that we don't
> need, especially if it has issues. I understand it's mandatory as per
> the spec, but just adding a struct here doesn't fix this or makes this
> better.
Sure, I can remove the unused structs, especially if they have issues. But I don't
see what issues they have, would you mind expanding on that?
Thanks,
Alex
