Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 4 Nov 2016 00:53:06 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> 
> 
> On 11/04/2016 12:49 AM, Igor Mammedov wrote:
> > On Fri, 4 Nov 2016 00:17:00 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >
> >>
> >>
> >> On 11/04/2016 12:13 AM, Igor Mammedov wrote:
> >>> On Thu, 3 Nov 2016 22:53:43 +0800
> >>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
> >>>>> On Thu, 3 Nov 2016 21:02:22 +0800
> >>>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>>>> just drop this and describe properly 'len' in spec section
> >>>>>>>>> i.e. len: length of entire returned data (including the
> >>>>>>>>> header)
> >>>>>>>>
> >>>>>>>> Okay, i will change the spec like this:
> >>>>>>>>
> >>>>>>>>     QEMU Writes Output Data (based on the offset in the
> >>>>>>>> page): [0x0 - 0x3]: 4 bytes, length of entire returned data
> >>>>>>>> (including the header)
> >>>>>>>>
> >>>>>>>> And drop the length field in Read_Fit return buffer, doc
> >>>>>>>> the fit buffer like this:
> >>>>>>>>
> >>>>>>>>     +----------+--------+--------+-------------------------------------------+
> >>>>>>>>     |  Field   | Length | Offset |
> >>>>>>>> Description               |
> >>>>>>>> +----------+--------+--------+-------------------------------------------+
> >>>>>>> you need to add length here, otherwise this table is not
> >>>>>>> correct
> >>>>>>
> >>>>>> Ah, so i am confused.
> >>>>>>
> >>>>>> struct NvdimmFuncReadFITOut definition is based on the layout
> >>>>>> of Read_FI output. You suggested to drop the length filed in
> >>>>>> NvdimmFuncReadFITOut but keep it in the layout, it is not
> >>>>>> consistent.
> >>>>>>
> >>>>>> I missed something?
> >>>>>
> >>>>> +struct NvdimmFuncReadFITOut {
> >>>>> +    /* the size of buffer filled by QEMU. */
> >>>>> +    uint32_t len;
> >>>>> +    uint32_t func_ret_status; /* return status code. */
> >>>>> +    uint8_t fit[0]; /* the FIT data. */
> >>>>> +} QEMU_PACKED;
> >>>>>
> >>>>> --------------------------------
> >>>>> | field       | len | off | desc...
> >>>>> --------------------------------
> >>>>> | length      |  4  |  0  | ....
> >>>>> --------------------------------
> >>>>> | status      |  4  |  4  | ....
> >>>>> --------------------------------
> >>>>> | fit data    | ................
> >>>>>
> >>>>> i.e. you were forgetting to add length in spec so offsets were
> >>>>> wrong even for described fields.
> >>>>
> >>>>
> >>>> We can not do this.
> >>>>
> >>>> @len is used by QEMU emulation to count the size of the buffer
> >>>> that _DSM should return. It's only used in NVDIMM_COMMON_DSM
> >>>> method which is shared by the DSM method from VM and Read_Fit.
> >>> spec describes buffer layout independently from AML that uses it,
> >>> so it should describe whole data structure.
> >>>
> >>> Then it's upto guest how to read this data, it could be QEMU
> >>> generated AML (as it's here) or some other driver or even BIOS.
> >>
> >> However, what we are talking about is Read_FIT method, so this is
> >> the layout of Read_FIT output rather than all memory in the dsm
> >> page.
> >>
> >> Actually, in the spec we already have documented the common len
> >> field:
> >>
> >>     QEMU Writes Output Data (based on the offset in the page):
> >>     [0x0 - 0x3]: 4 bytes, the length of result
> >>     [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU
> >>
> >> Also, i really do not hope to use this field to count the buffer
> >> size returned by Read_FIT, we'd try the best to reuse existing DSM
> >> method (NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM
> >> method.
> > buffer layout describes interface between QEMU and firmware (AML)
> > and it should describe it completely every time to avoid confusion.
> >
> > See for example ACPI spec, NFIT table, SRAT table, ...
> > all table descriptions start with complete header.
> 
> Okay. Understood. :)
> 
> >
> > If you skip length it rises question how much fit data are there,
> > meaning interface description isn't complete.
> 
> So how about introduce a length field in the output buffer just
> as this patch did? I understand we are able to count the size
> from dsm len, however, it can simplify the code a lot...
it's redundant as there already is length for whole buffer,
interface should be kept as simple as possible and not include
redundant data for convenience.

> 
> >
> > if you want to describe AML there you can do it after interface
> > description saying that common NCAL method extracts status and fit
> > data form dsm page and returns that as buffer object, but it's AML
> > impl. specific. I could write an alternative AML code that would
> > deal with dms page in its own way as long as I would know what
> > write/read at what offset.
> 
> Yes, i agree with you.
>