* [PATCH V2] ieee1275/ofdisk: vscsi lun handling on lun len
@ 2023-11-27 12:37 Mukesh Kumar Chaurasiya
2023-11-29 19:15 ` Daniel Kiper
0 siblings, 1 reply; 5+ messages in thread
From: Mukesh Kumar Chaurasiya @ 2023-11-27 12:37 UTC (permalink / raw)
To: grub-devel
Cc: meghanaprakash, avnish, brking, mamatha4, mchauras,
Mukesh Kumar Chaurasiya
The information about "vscsi-report-luns" data is a list of disk details
with pairs of memory addresses and lengths.
8 bytes 8 bytes
lun-addr ---> ------------------------ 8 bytes
^ | buf-addr | buf-len | --------> -------------
| ------------------------ | lun |
| | buf-addr | buf-len | ----| -------------
"len" ------------------------ | | ... |
| | ... | | -------------
| ------------------------ | | lun |
| | buf-addr | buf-len | | -------------
V ------------------------ |
|---> -------------
| lun |
-------------
| ... |
-------------
| lun |
-------------
The way the expression "(args.table + 4 + 8 * i)" is used is incorrect and
can be confusing. The list of LUNs doesn't end with NULL, indicated by
"while (*ptr)". Usually, this loop doesn't process any LUNs because it ends
before checking any as first reported LUN is likely to be "0". The list of
LUNs ends based on its length, not by a NULL value.
Signed-off-by: Mukesh Kumar Chaurasiya <mchauras@linux.ibm.com>
---
grub-core/disk/ieee1275/ofdisk.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
index c6cba0c8a..9cd8898f1 100644
--- a/grub-core/disk/ieee1275/ofdisk.c
+++ b/grub-core/disk/ieee1275/ofdisk.c
@@ -222,8 +222,12 @@ dev_iterate (const struct grub_ieee1275_devalias *alias)
grub_ieee1275_cell_t table;
}
args;
+ struct lun_buf {
+ grub_uint64_t *buf_addr;
+ grub_uint64_t buf_len;
+ } *tbl;
char *buf, *bufptr;
- unsigned i;
+ unsigned int i, j;
if (grub_ieee1275_open (alias->path, &ihandle))
return;
@@ -248,12 +252,13 @@ dev_iterate (const struct grub_ieee1275_devalias *alias)
return;
bufptr = grub_stpcpy (buf, alias->path);
+ tbl = (struct lun_len *) args.table;
for (i = 0; i < args.nentries; i++)
{
grub_uint64_t *ptr;
- ptr = *(grub_uint64_t **) (args.table + 4 + 8 * i);
- while (*ptr)
+ ptr = (grub_uint64_t *)(grub_addr_t) tbl[i].buf_addr;
+ for (j = 0; j < tbl[i].buf_len; j++)
{
grub_snprintf (bufptr, 32, "/disk@%" PRIxGRUB_UINT64_T, *ptr++);
dev_iterate_real (buf, buf);
--
2.43.0
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH V2] ieee1275/ofdisk: vscsi lun handling on lun len
2023-11-27 12:37 [PATCH V2] ieee1275/ofdisk: vscsi lun handling on lun len Mukesh Kumar Chaurasiya
@ 2023-11-29 19:15 ` Daniel Kiper
2024-01-17 7:09 ` Mukesh Kumar Chaurasiya
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Kiper @ 2023-11-29 19:15 UTC (permalink / raw)
To: The development of GNU GRUB
Cc: meghanaprakash, avnish, brking, mamatha4, mchauras,
Mukesh Kumar Chaurasiya
On Mon, Nov 27, 2023 at 06:07:42PM +0530, Mukesh Kumar Chaurasiya wrote:
> The information about "vscsi-report-luns" data is a list of disk details
> with pairs of memory addresses and lengths.
Not lengths but counts of entries.
> 8 bytes 8 bytes
> lun-addr ---> ------------------------ 8 bytes
> ^ | buf-addr | buf-len | --------> -------------
> | ------------------------ | lun |
> | | buf-addr | buf-len | ----| -------------
> "len" ------------------------ | | ... |
> | | ... | | -------------
> | ------------------------ | | lun |
> | | buf-addr | buf-len | | -------------
> V ------------------------ |
> |---> -------------
> | lun |
> -------------
> | ... |
> -------------
> | lun |
> -------------
> The way the expression "(args.table + 4 + 8 * i)" is used is incorrect and
I would drop quotation marks from here.
> can be confusing. The list of LUNs doesn't end with NULL, indicated by
> "while (*ptr)". Usually, this loop doesn't process any LUNs because it ends
> before checking any as first reported LUN is likely to be "0". The list of
... ditto...
> LUNs ends based on its length, not by a NULL value.
>
> Signed-off-by: Mukesh Kumar Chaurasiya <mchauras@linux.ibm.com>
> ---
> grub-core/disk/ieee1275/ofdisk.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
> index c6cba0c8a..9cd8898f1 100644
> --- a/grub-core/disk/ieee1275/ofdisk.c
> +++ b/grub-core/disk/ieee1275/ofdisk.c
> @@ -222,8 +222,12 @@ dev_iterate (const struct grub_ieee1275_devalias *alias)
> grub_ieee1275_cell_t table;
> }
> args;
> + struct lun_buf {
> + grub_uint64_t *buf_addr;
This will not work on 32-bit architectures.
> + grub_uint64_t buf_len;
Again, this is not length. It is number of entries.
> + } *tbl;
Why do you define this struct here? Do not you have its proper
definition in currently existing code? Even if not I would define it
properly outside of this function because somebody may need it later.
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH V2] ieee1275/ofdisk: vscsi lun handling on lun len
2023-11-29 19:15 ` Daniel Kiper
@ 2024-01-17 7:09 ` Mukesh Kumar Chaurasiya
2024-01-23 18:02 ` Daniel Kiper
0 siblings, 1 reply; 5+ messages in thread
From: Mukesh Kumar Chaurasiya @ 2024-01-17 7:09 UTC (permalink / raw)
To: Daniel Kiper, The development of GNU GRUB
Cc: meghanaprakash, avnish, brking, mamatha4, mchauras
On 11/30/23 00:45, Daniel Kiper wrote:
> On Mon, Nov 27, 2023 at 06:07:42PM +0530, Mukesh Kumar Chaurasiya wrote:
>> Signed-off-by: Mukesh Kumar Chaurasiya <mchauras@linux.ibm.com>
>> ---
>> grub-core/disk/ieee1275/ofdisk.c | 11 ++++++++---
>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
>> index c6cba0c8a..9cd8898f1 100644
>> --- a/grub-core/disk/ieee1275/ofdisk.c
>> +++ b/grub-core/disk/ieee1275/ofdisk.c
>> @@ -222,8 +222,12 @@ dev_iterate (const struct grub_ieee1275_devalias *alias)
>> grub_ieee1275_cell_t table;
>> }
>> args;
>> + struct lun_buf {
>> + grub_uint64_t *buf_addr;
> This will not work on 32-bit architectures.
Umm...
well i checked this and the definition for 32bit and 64bit are same.
So the structure part should be fine.
+ ptr =(grub_uint64_t*)(grub_addr_t) tbl[i].buf_addr;
and this should handle the 32bit arch in terms of address.
for rest i'll send out a new version of patch
Regards,
Mukesh
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH V2] ieee1275/ofdisk: vscsi lun handling on lun len
2024-01-17 7:09 ` Mukesh Kumar Chaurasiya
@ 2024-01-23 18:02 ` Daniel Kiper
2024-01-24 17:11 ` Mukesh Kumar Chaurasiya
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Kiper @ 2024-01-23 18:02 UTC (permalink / raw)
To: Mukesh Kumar Chaurasiya
Cc: The development of GNU GRUB, meghanaprakash, avnish, brking,
mamatha4, mchauras
On Wed, Jan 17, 2024 at 12:39:06PM +0530, Mukesh Kumar Chaurasiya wrote:
> On 11/30/23 00:45, Daniel Kiper wrote:
> > On Mon, Nov 27, 2023 at 06:07:42PM +0530, Mukesh Kumar Chaurasiya wrote:
> > > Signed-off-by: Mukesh Kumar Chaurasiya <mchauras@linux.ibm.com>
> > > ---
> > > grub-core/disk/ieee1275/ofdisk.c | 11 ++++++++---
> > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
> > > index c6cba0c8a..9cd8898f1 100644
> > > --- a/grub-core/disk/ieee1275/ofdisk.c
> > > +++ b/grub-core/disk/ieee1275/ofdisk.c
> > > @@ -222,8 +222,12 @@ dev_iterate (const struct grub_ieee1275_devalias *alias)
> > > grub_ieee1275_cell_t table;
> > > }
> > > args;
> > > + struct lun_buf {
> > > + grub_uint64_t *buf_addr;
> > This will not work on 32-bit architectures.
>
> Umm...
>
> well i checked this and the definition for 32bit and 64bit are same. So the
I am not entirely sure what you mean by "the same" here.
> structure part should be fine.
>
> + ptr =(grub_uint64_t*)(grub_addr_t) tbl[i].buf_addr;
> and this should handle the 32bit arch in terms of address.
> for rest i'll send out a new version of patch
The problem is with the *buf_addr pointer. Its size on 32-bit platform
is 32-bit. So, on 32-bit platform you will have struct with 32-bit member and
64-bit member following it. This is not what you expect. The buf_addr/buf_size
table has each entry with two 64-bit members. Am I missing something?
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH V2] ieee1275/ofdisk: vscsi lun handling on lun len
2024-01-23 18:02 ` Daniel Kiper
@ 2024-01-24 17:11 ` Mukesh Kumar Chaurasiya
0 siblings, 0 replies; 5+ messages in thread
From: Mukesh Kumar Chaurasiya @ 2024-01-24 17:11 UTC (permalink / raw)
To: Daniel Kiper
Cc: The development of GNU GRUB, meghanaprakash, avnish, brking,
mamatha4, mchauras
On 1/23/24 23:32, Daniel Kiper wrote:
> On Wed, Jan 17, 2024 at 12:39:06PM +0530, Mukesh Kumar Chaurasiya wrote:
>> On 11/30/23 00:45, Daniel Kiper wrote:
>>> On Mon, Nov 27, 2023 at 06:07:42PM +0530, Mukesh Kumar Chaurasiya wrote:
>>>> Signed-off-by: Mukesh Kumar Chaurasiya <mchauras@linux.ibm.com>
>>>> ---
>>>> grub-core/disk/ieee1275/ofdisk.c | 11 ++++++++---
>>>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
>>>> index c6cba0c8a..9cd8898f1 100644
>>>> --- a/grub-core/disk/ieee1275/ofdisk.c
>>>> +++ b/grub-core/disk/ieee1275/ofdisk.c
>>>> @@ -222,8 +222,12 @@ dev_iterate (const struct grub_ieee1275_devalias *alias)
>>>> grub_ieee1275_cell_t table;
>>>> }
>>>> args;
>>>> + struct lun_buf {
>>>> + grub_uint64_t *buf_addr;
>>> This will not work on 32-bit architectures.
>> Umm...
>>
>> well i checked this and the definition for 32bit and 64bit are same. So the
> I am not entirely sure what you mean by "the same" here.
>
>> structure part should be fine.
>>
>> + ptr =(grub_uint64_t*)(grub_addr_t) tbl[i].buf_addr;
>> and this should handle the 32bit arch in terms of address.
>> for rest i'll send out a new version of patch
> The problem is with the *buf_addr pointer. Its size on 32-bit platform
> is 32-bit. So, on 32-bit platform you will have struct with 32-bit member and
> 64-bit member following it. This is not what you expect. The buf_addr/buf_size
> table has each entry with two 64-bit members. Am I missing something?
>
> Daniel
In case of 32 bit the upper 4 bytes are padded with 00. I am not sure
why, this was weird for me too.
Regards
Mukesh
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-01-24 17:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-27 12:37 [PATCH V2] ieee1275/ofdisk: vscsi lun handling on lun len Mukesh Kumar Chaurasiya
2023-11-29 19:15 ` Daniel Kiper
2024-01-17 7:09 ` Mukesh Kumar Chaurasiya
2024-01-23 18:02 ` Daniel Kiper
2024-01-24 17:11 ` Mukesh Kumar Chaurasiya
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.