Re: [Qemu-devel] [qemu-s390x] [PATCH v6 01/12] s390-ccw: refactor boot map table code

[Thomas Huth <thuth@redhat.com>]

On 16.02.2018 15:57, Collin L. Walling wrote:
> On 02/16/2018 07:19 AM, Viktor Mihajlovski wrote:
>> On 16.02.2018 11:42, Thomas Huth wrote:
>>> On 15.02.2018 23:54, Collin L. Walling wrote:
>>>> Some ECKD bootmap code was using structs designed for SCSI.
>>>> Even though this works, it confuses readability. Add a new
>>>> BootMapTable struct to assist with readability in bootmap
>>>> entry code. Also:
>>>>
>>>> - replace ScsiMbr in ECKD code with appropriate structs
>>>> - fix read_block messages to reflect BootMapTable
>>>> - fixup ipl_scsi to use BootMapTable (referred to as Program Table)
>>>> - defined value for maximum table entries
>>>>
>>>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>>>> ---
>>>>   pc-bios/s390-ccw/bootmap.c | 60
>>>> +++++++++++++++++++++-------------------------
>>>>   pc-bios/s390-ccw/bootmap.h | 14 +++++++++--
>>>>   2 files changed, 39 insertions(+), 35 deletions(-)
>>> [...]
>>>> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
>>>> index cf99a4c..850b655 100644
>>>> --- a/pc-bios/s390-ccw/bootmap.h
>>>> +++ b/pc-bios/s390-ccw/bootmap.h
>>>> @@ -53,6 +53,15 @@ typedef union BootMapPointer {
>>>>       ExtEckdBlockPtr xeckd;
>>>>   } __attribute__ ((packed)) BootMapPointer;
>>>>   +#define MAX_TABLE_ENTRIES  30
>>>> +
>>>> +/* aka Program Table */
>>>> +typedef struct BootMapTable {
>>>> +    uint8_t magic[4];
>>>> +    uint8_t reserved[12];
>>>> +    BootMapPointer entry[];
>>>> +} __attribute__ ((packed)) BootMapTable;
>>>> +
>>>>   typedef struct ComponentEntry {
>>>>       ScsiBlockPtr data;
>>>>       uint8_t pad[7];
>>>> @@ -69,8 +78,9 @@ typedef struct ComponentHeader {
>>>>   typedef struct ScsiMbr {
>>>>       uint8_t magic[4];
>>>>       uint32_t version_id;
>>>> -    uint8_t reserved[8];
>>>> -    ScsiBlockPtr blockptr[];
>>>> +    uint8_t reserved1[8];
>>>> +    ScsiBlockPtr pt;   /* block pointer to program table */
>>>> +    uint8_t reserved2[120];
>>> Did you want to pad the struct to 512 bytes here? If so, I think that
>>> should have been "uint32_t" instead of "uint8_t" or "480" instead of
>>> "120" ?
>>>
>> Should probably be uint8_t[480] then to stay in style with other
>> reserved field definitions.
> 
> The idea was to mimic the Scsi MBR struct defined in zipl.  It has
> reserved space for what would appear to be for 5 more ScsiBlockPtrs
> (which are not used in zipl) and a boot_info struct (which we don't need
> in QEMU).
> 
> Alternatively, I can just omit the reserved2 field. I placed it there
> for "completeness" but it is not at all necessary.

I'd say just drop it. It looks rather confusing than helpful to me.

 Thomas