[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv5 5/5] fw_cfg: move QOM type defines and fw_cfg
From: |
Mark Cave-Ayland |
Subject: |
Re: [Qemu-devel] [PATCHv5 5/5] fw_cfg: move QOM type defines and fw_cfg types into fw_cfg.h |
Date: |
Mon, 19 Jun 2017 13:43:32 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 |
On 19/06/17 09:57, Laszlo Ersek wrote:
> On 06/18/17 22:23, Michael S. Tsirkin wrote:
>> On Sun, Jun 18, 2017 at 09:02:14AM +0100, Mark Cave-Ayland wrote:
>>> By exposing FWCfgIoState and FWCfgMemState internals we allow the
>>> possibility
>>> for the internal MemoryRegion fields to be mapped by name for boards that
>>> wish
>>> to wire up the fw_cfg device themselves.
>>>
>>> An additional minor tidy-up is that the FWCfgEntry typedef is moved from the
>>> struct definitions in fw_cfg.h to typedefs.h along with the others.
>
> I think this paragraph should be dropped.
No problem, I'll do that for the follow-up v6 patchset.
>>>
>>> Signed-off-by: Mark Cave-Ayland <address@hidden>
>>> ---
>>> hw/nvram/fw_cfg.c | 55 ------------------------------------------
>>> include/hw/nvram/fw_cfg.h | 58
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>> include/qemu/typedefs.h | 1 +
>>> 3 files changed, 59 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>>> index df99903..00771c9 100644
>>> --- a/hw/nvram/fw_cfg.c
>>> +++ b/hw/nvram/fw_cfg.c
>>> @@ -40,14 +40,6 @@
>>> #define FW_CFG_NAME "fw_cfg"
>>> #define FW_CFG_PATH "/machine/" FW_CFG_NAME
>>>
>>> -#define TYPE_FW_CFG "fw_cfg"
>>> -#define TYPE_FW_CFG_IO "fw_cfg_io"
>>> -#define TYPE_FW_CFG_MEM "fw_cfg_mem"
>>> -
>>> -#define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG)
>>> -#define FW_CFG_IO(obj) OBJECT_CHECK(FWCfgIoState, (obj), TYPE_FW_CFG_IO)
>>> -#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
>>> -
>>> /* FW_CFG_VERSION bits */
>>> #define FW_CFG_VERSION 0x01
>>> #define FW_CFG_VERSION_DMA 0x02
>>> @@ -61,53 +53,6 @@
>>>
>>> #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */
>>>
>>> -typedef struct FWCfgEntry {
>>> - uint32_t len;
>>> - bool allow_write;
>>> - uint8_t *data;
>>> - void *callback_opaque;
>>> - FWCfgReadCallback read_callback;
>>> -} FWCfgEntry;
>>
>> This still doesn't seem to do what Laszlo requested which is to keep
>> as many types and macros as possible in fw_cfg.c, only put typedefs in
>> fw_cfg.h.
>
> Sort of; what's missing from this version (for me anyway) is that the
> internals of FWCfgEntry should remain in the C file, because we never
> depend on those fields -- or the size of that structure -- externally.
> I'm OK with the rest.
>
> Mark, can you please squash the following diff into this patch -- this
> is what would implement my request (2) in
> <https://www.mail-archive.com/address@hidden/msg458313.html>:
>
>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>> index b0511b9a9d77..b77ea48abb1d 100644
>> --- a/include/hw/nvram/fw_cfg.h
>> +++ b/include/hw/nvram/fw_cfg.h
>> @@ -46,14 +46,6 @@ typedef struct FWCfgDmaAccess {
>>
>> typedef void (*FWCfgReadCallback)(void *opaque);
>>
>> -struct FWCfgEntry {
>> - uint32_t len;
>> - bool allow_write;
>> - uint8_t *data;
>> - void *callback_opaque;
>> - FWCfgReadCallback read_callback;
>> -};
>> -
>> struct FWCfgState {
>> /*< private >*/
>> SysBusDevice parent_obj;
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 00771c98505c..9b0aaa21a202 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -53,6 +53,14 @@
>>
>> #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */
>>
>> +struct FWCfgEntry {
>> + uint32_t len;
>> + bool allow_write;
>> + uint8_t *data;
>> + void *callback_opaque;
>> + FWCfgReadCallback read_callback;
>> +};
>> +
>> #define JPG_FILE 0
>> #define BMP_FILE 1
>>
>
> As I wrote in the msg linked above, 'FWCfgEntry need not be moved from
> the C file to the header file [...] it's fine to leave FWCfgEntry an
> incomplete type in "fw_cfg.h"'.
Yes, that's totally my fault as I misinterpreted your comment from your
previous email - sorry about that.
> With the above two changes (commit message and code update) squashed
> into patch #5:
>
> Reviewed-by: Laszlo Ersek <address@hidden>
>
> and also for the series:
>
> Tested-by: Laszlo Ersek <address@hidden>
>
> (I tested the IO mapped variant with OVMF, exercising fw_cfg DMA and
> fw_cfg write, and the MMIO mapped variant with ArmVirtQemu (aka
> "AAVMF"), exercising fw_cfg DMA.)
Fantastic! It's good to know that the changes don't cause any
regressions for both DMA operations and MMIO fw_cfg instances.
I'll squash your diff into patch 5, update the tags and then re-post a
v6 shortly.
Assuming that this is the final revision, who is the right person to
accept the patchset into their queue for merge upstream?
ATB,
Mark.